pkg/backup/dependencies/registry.go (1)

31-41: Register can leave the registry in a partially-modified state on error.

If called directly (not via NewRegistry), passing e.g. [A, nil, B] will append A before returning the nil-resolver error, leaving the registry with A registered but B missing. Consider validating all resolvers upfront before mutating state, so the operation is atomic.

♻️ Suggested fix: validate-then-append

 func (r *Registry) Register(resolvers ...Resolver) error {
+	for _, resolver := range resolvers {
+		if resolver == nil {
+			return errors.New("cannot register nil resolver")
+		}
+	}
+
 	for _, resolver := range resolvers {
-		if resolver == nil {
-			return errors.New("cannot register nil resolver")
-		}
-
 		r.resolvers = append(r.resolvers, resolver)
 	}
 
 	return nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/backup/dependencies/registry.go` around lines 31 - 41, The Register
method currently mutates r.resolvers as it iterates and returns an error if any
resolver is nil, which can leave the registry partially updated; update Register
to first validate all provided resolvers (iterate resolvers and return error if
any is nil) before mutating state, or build a temporary slice of validated
resolvers and append that to r.resolvers only after validation succeeds
(referencing the Register method, the resolver loop and r.resolvers append).

pkg/backup/command.go (1)

99-116: Consider collapsing the two branches into a single NewRegistry call.

Both branches do the same thing — construct a registry and assign it — differing only in whether resolvers are passed. This can be simplified:

♻️ Suggested simplification

-	if c.Dependencies {
-		registry, err := dependencies.NewRegistry(
-			notebooks.NewResolver(),
-			dspa.NewResolver(),
-		)
-		if err != nil {
-			return fmt.Errorf("registering dependency resolvers: %w", err)
-		}
-
-		c.depRegistry = registry
-	} else {
-		registry, err := dependencies.NewRegistry()
-		if err != nil {
-			return fmt.Errorf("creating dependency registry: %w", err)
-		}
-
-		c.depRegistry = registry
+	var resolvers []dependencies.Resolver
+	if c.Dependencies {
+		resolvers = append(resolvers, notebooks.NewResolver(), dspa.NewResolver())
+	}
+
+	registry, err := dependencies.NewRegistry(resolvers...)
+	if err != nil {
+		return fmt.Errorf("creating dependency registry: %w", err)
 	}
+
+	c.depRegistry = registry

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/backup/command.go` around lines 99 - 116, The two branches around
c.Dependencies can be collapsed: build the resolver args conditionally and call
dependencies.NewRegistry once, then assign to c.depRegistry. Specifically,
prepare a slice or variadic list containing notebooks.NewResolver() and
dspa.NewResolver() only when c.Dependencies is true, call
dependencies.NewRegistry(...) with that list, handle the error, and assign the
returned registry to c.depRegistry; keep the existing error messages but remove
the duplicated registry creation logic in the branches.

pkg/migrate/command_run.go (2)

39-44: Duplicated registry initialization across all three command constructors.

NewRunCommand, NewPrepareCommand, and NewListCommand all contain the identical registry creation block:

registry, err := action.NewActionRegistry(
    &rhbok.RHBOKMigrationAction{},
)

Consider extracting a shared helper (e.g., newDefaultActionRegistry() (*action.ActionRegistry, error)) in the migrate package to keep the action list in one place. If a new action is added, only one site needs updating.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/migrate/command_run.go` around lines 39 - 44, The three constructors
NewRunCommand, NewPrepareCommand, and NewListCommand all duplicate the registry
initialization using action.NewActionRegistry(&rhbok.RHBOKMigrationAction{});
extract this into a single helper in the migrate package (e.g.,
newDefaultActionRegistry() (*action.ActionRegistry, error)) that returns the
configured registry, then replace the duplicated blocks in NewRunCommand,
NewPrepareCommand, and NewListCommand with calls to newDefaultActionRegistry();
ensure the helper centralizes the action list so future additions are made in
one place.

---

73-80: Duplicated version-parsing block across commands.

The TargetVersion → parsedTargetVersion parsing via semver.ParseTolerant is duplicated in RunCommand.Complete(), PrepareCommand.Complete(), and ListCommand.Complete(). Given that this PR introduces version.SemVersion (which encapsulates exactly this pattern), consider adopting it in the migrate commands as well — or at least extracting a shared helper in SharedOptions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/migrate/command_run.go` around lines 73 - 80, The
TargetVersion→parsedTargetVersion parsing logic is duplicated across
RunCommand.Complete(), PrepareCommand.Complete(), and ListCommand.Complete();
replace the repeated semver.ParseTolerant usage by reusing the new
version.SemVersion type or add a helper on SharedOptions to centralize parsing.
Specifically, either change the commands to store and use a version.SemVersion
(construct it from TargetVersion) or implement a
SharedOptions.ParseTargetVersion() method that calls
semver.ParseTolerant(TargetVersion) and returns/sets the parsedVersion (used
instead of parsedTargetVersion), then update RunCommand, PrepareCommand, and
ListCommand to call that helper rather than duplicating the block. Ensure errors
are returned the same way and preserve the behavior of accepting partial
versions.

pkg/migrate/action/registry.go (1)

27-41: Partial registration on duplicate ID.

If Register is called with multiple actions and a duplicate ID is encountered mid-slice, the actions before the duplicate will have already been inserted into the map while the rest are skipped. For the current usage (fresh registry in NewActionRegistry), this is benign. But if Register is ever called independently to add a batch of actions, the caller gets an error with the registry in a partially-mutated state.

Consider either rolling back on error or validating all IDs first before inserting.

♻️ Validate-then-insert approach

 func (r *ActionRegistry) Register(actions ...Action) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
+	// Validate all IDs first to avoid partial registration
+	for _, a := range actions {
+		id := a.ID()
+		if _, exists := r.actions[id]; exists {
+			return fmt.Errorf("action with ID %q already registered", id)
+		}
+	}
+
 	for _, a := range actions {
-		id := a.ID()
-		if _, exists := r.actions[id]; exists {
-			return fmt.Errorf("action with ID %q already registered", id)
-		}
-
-		r.actions[id] = a
+		r.actions[a.ID()] = a
 	}
 
 	return nil
 }

Note: this also doesn't catch duplicates within the batch itself (two actions in the same call with the same ID). If that matters, you'd need to check for intra-batch duplicates as well.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/migrate/action/registry.go` around lines 27 - 41, The Register method on
ActionRegistry currently inserts actions as it iterates, causing partial
registration if a duplicate ID is found; change Register (method on
ActionRegistry) to a validate-then-insert flow: while holding r.mu, first
iterate over the provided actions and collect their IDs via Action.ID(),
checking against r.actions for existing IDs and against a temporary local set to
detect intra-batch duplicates, and if any duplicate is found return an error
without mutating r.actions; only after validation passes, iterate again to
insert each action into r.actions; keep the same locking (r.mu.Lock()/defer
r.mu.Unlock()) around both validation and insertion so the state remains
consistent.

pkg/lint/check/registry.go (1)

30-43: Register can leave the registry partially populated on duplicate-ID error.

If the third check out of five has a duplicate ID, the first two are already inserted. After the error is returned, the registry retains those partial additions. This doesn't matter for NewRegistry (which discards the whole registry on error), but callers using Register directly on a live registry will observe a half-applied batch.

Consider either pre-validating all IDs before inserting, or documenting that on error the registry may contain a subset of the provided checks.

Pre-validate to make Register atomic

 func (r *CheckRegistry) Register(checks ...Check) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
+	// Pre-validate: no duplicates among existing or within the batch
+	for _, c := range checks {
+		if _, exists := r.checks[c.ID()]; exists {
+			return fmt.Errorf("check with ID %s already registered", c.ID())
+		}
+	}
+
 	for _, c := range checks {
-		if _, exists := r.checks[c.ID()]; exists {
-			return fmt.Errorf("check with ID %s already registered", c.ID())
-		}
-
 		r.checks[c.ID()] = c
 	}
 
 	return nil
 }

Note: this simple version doesn't catch duplicates within the batch itself. You'd need a seen-set for that.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/lint/check/registry.go` around lines 30 - 43, The Register method can
partially apply a batch when a duplicate ID is encountered; make it atomic by
first validating all incoming checks before mutating r.checks: iterate over the
checks to ensure none of their IDs collide with existing entries in r.checks and
none duplicate within the batch (use a local seen set keyed by c.ID()), return
an error if any conflict found, and only after this validation loop acquire r.mu
and insert all checks into r.checks; keep function and symbol names exactly as
CheckRegistry.Register, r.checks, r.mu and c.ID() to locate the code.

pkg/lint/command_options.go (1)

209-216: WithTargetVersion panics on invalid input, contradicting the PR's goal of removing panics.

The PR removes MustRegister panics from registry constructors in favor of error returns, but this option introduces a new panic path. Since NewCommand already returns (*Command, error), the option could return an error instead. A caller passing WithTargetVersion("not-semver") will get a panic rather than a graceful error.

Consider changing CommandOption to return an error, or using a recover/error pattern:

Option A: Error-returning CommandOption

-type CommandOption func(*Command)
+type CommandOption func(*Command) error

 func WithTargetVersion(v string) CommandOption {
-	return func(c *Command) {
-		if err := c.TargetVersion.Set(v); err != nil {
-			panic(fmt.Sprintf("WithTargetVersion: %v", err))
-		}
+	return func(c *Command) error {
+		if err := c.TargetVersion.Set(v); err != nil {
+			return fmt.Errorf("WithTargetVersion: %w", err)
+		}
+		return nil
 	}
 }

Then in NewCommand, apply options with error checking:

for _, opt := range options {
    if err := opt(cmd); err != nil {
        return nil, err
    }
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/lint/command_options.go` around lines 209 - 216, WithTargetVersion
currently panics on invalid semver via TargetVersion.Set, which reintroduces
panics; change the CommandOption type to return error (e.g., func(*Command)
error), update WithTargetVersion to return an error instead of panicking (call
c.TargetVersion.Set(v) and return that error wrapped with context), and update
NewCommand to apply options with error checking (iterate options, call each, and
if any returns error, return nil and that error). Ensure all other CommandOption
implementations are updated to the new signature and any callers of NewCommand
continue to handle the returned error.

cmd/migrate/run/run.go (1)

53-58: Silent subcommand omission on initialization failure.

If NewRunCommand fails, the "run" subcommand silently disappears from the CLI. The error is printed to stderr, but the parent command continues without it. A user running kubectl odh migrate run ... would get an "unknown command" error instead of the actual initialization failure.

This is consistent with the pattern used in cmd/lint/lint.go and other sibling commands, so it's acceptable as-is. Just noting that a more user-friendly approach might be to register a stub command that always returns the initialization error, so users get a clear message when they try to use it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/migrate/run/run.go` around lines 53 - 58, If
migrate.NewRunCommand(streams) returns an error the "run" subcommand is omitted,
causing "unknown command" for users; instead, catch the error and
create/register a stub Cobra command named "run" that always returns or prints
the initialization error when invoked (use the same command name and help text),
so callers of the run subcommand see the original initialization error;
reference migrate.NewRunCommand and the parent command registration flow and use
parent.ErrOrStderr() or parent.AddCommand to attach the stub command.

docs/lint/writing-checks.md (1)

136-154: Check counts now align with the actual code.

The counts (Components 13, Dependencies 2, Workloads 13) match the registrations in pkg/lint/command.go. The previous review's concern about mismatched counts is resolved.

Minor nit: the example check names shown here (e.g., codeflare.NewRemovalCheck(), datasciencepipelines.NewInstructLabRemovalCheck()) don't exactly match the real code (raycomponent.NewCodeFlareRemovalCheck(), datasciencepipelines.NewRenamingCheck()), but since this is an abbreviated illustrative example, it's acceptable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/lint/writing-checks.md` around lines 136 - 154, Update the illustrative
example check names to match the real exported constructors used in code so the
docs mirror reality: replace codeflare.NewRemovalCheck() with
raycomponent.NewCodeFlareRemovalCheck() and
datasciencepipelines.NewInstructLabRemovalCheck() with
datasciencepipelines.NewRenamingCheck() (and any other mismatched example names)
in the registry example so the symbols shown (e.g.,
raycomponent.NewCodeFlareRemovalCheck, datasciencepipelines.NewRenamingCheck,
certmanager.NewCheck, guardrails.NewOtelMigrationCheck) match the actual
registrations in pkg/lint/command.go.

cmd/migrate/prepare/prepare.go (1)

55-87: Addresses the previously raised initialization-error swallowing issue.

AddCommand now returns error and properly wraps the failure from NewPrepareCommand, fully resolving the concern about silent subcommand drop.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/migrate/prepare/prepare.go` around lines 55 - 87, The
parent.AddCommand(cmd) call is currently ignored; since AddCommand now returns
an error, check its return and wrap it with context so failures (including those
originating from migrate.NewPrepareCommand) aren't silently dropped. After
creating cmd and before returning, replace the bare parent.AddCommand(cmd) with
an error check that returns fmt.Errorf("adding prepare subcommand: %w", err) on
failure; keep command.AddFlags(cmd.Flags()) and the existing RunE logic
unchanged.

pkg/migrate/action/registry.go (1)

27-41: Partial registration on duplicate detection.

If Register(a, b, c) is called and b has a duplicate ID, a is already inserted into the map before the error is returned. This leaves the registry in a partially-modified state. In practice this is safe when called from NewActionRegistry (the entire registry is discarded on error), but direct Register calls could observe a half-applied batch.

A pre-validation pass or rollback would avoid this, though the same pattern exists in the sibling registries so this is consistent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/migrate/action/registry.go` around lines 27 - 41, ActionRegistry.Register
currently mutates r.actions as it iterates, causing partial registration if a
later action conflicts; fix by validating the whole incoming batch first: inside
Register (use r.mu.Lock while validating) iterate the provided actions to
collect their IDs, check for duplicates within the batch and against existing
keys in r.actions (use a temporary map or set), return an error if any conflict
is found, and only after validation completes loop again to add them to
r.actions; this ensures atomic-like behavior for Register and avoids partial
state (refer to ActionRegistry.Register, r.actions and r.mu).

pkg/backup/dependencies/registry.go (1)

31-41: Partial registration on nil resolver in direct Register calls.

If Register is called directly (not via constructor) with e.g. [A, nil, B], resolver A is appended before the nil-check error is returned for the second element, leaving the registry in a partially mutated state. The constructor path is safe (returns nil on error), but direct callers of Register should be aware.

This matches the pattern in pkg/lint/check/registry.go, so it's consistent. Just flagging for awareness.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/backup/dependencies/registry.go` around lines 31 - 41, Register currently
appends each resolver as it iterates, so if a nil appears mid-slice the registry
is left partially mutated; change Registry.Register (method Register on type
Registry) to validate all incoming resolvers for nil first (or build into a
temporary slice) and only mutate r.resolvers after validation succeeds, e.g.,
iterate once to check for nil and return an error if found, then append the
whole slice to r.resolvers (or append the validated temp slice) to ensure atomic
registration.

pkg/lint/check/registry.go (1)

30-43: Register leaves the registry in a partial state when a batch call fails mid-iteration.

If Register(a, b, dup) is called on an existing registry and dup is a duplicate, a and b are already committed by the time the error is returned. For NewRegistry this is harmless (the registry is discarded via nil), but callers using Register directly with multiple checks get an inconsistently modified registry.

♻️ Two-pass (validate then insert) to make batch Register atomic

 func (r *CheckRegistry) Register(checks ...Check) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
+	// Validate all IDs first so the operation is all-or-nothing.
+	for _, c := range checks {
+		if _, exists := r.checks[c.ID()]; exists {
+			return fmt.Errorf("check with ID %s already registered", c.ID())
+		}
+	}
+
 	for _, c := range checks {
-		if _, exists := r.checks[c.ID()]; exists {
-			return fmt.Errorf("check with ID %s already registered", c.ID())
-		}
 		r.checks[c.ID()] = c
 	}
 
 	return nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/lint/check/registry.go` around lines 30 - 43, Register currently mutates
r.checks as it iterates and returns mid-loop on duplicates, leaving the registry
partially updated; make Register atomic by first validating all inputs (verify
none of checks' IDs exist in r.checks and detect duplicates within the incoming
checks slice) and only if validation passes perform a second loop to insert
them. Keep the mutex (r.mu) to protect both phases (or lock for validation then
insert under same lock) and return the existing error message when validation
fails so the registry remains unmodified on error; reference the
CheckRegistry.Register method and the checks slice/r.checks map when
implementing the two-pass validate-then-insert approach.

pkg/lint/command.go (1)

408-413: Dead code — IsSet() is always false in this call path.

formatAndOutputResults is called exclusively from runLintMode, which is entered only when c.TargetVersion.IsSet() is false (Line 183). The IsSet() block at Lines 410–413 can never be true, and targetVer is unconditionally nil here. Keeping this guard implies the method could be called in upgrade mode, which is misleading.

♻️ Proposed simplification

  clusterVer := &c.currentClusterVersion

- var targetVer *string
- if c.TargetVersion.IsSet() {
- 	s := c.TargetVersion.String()
- 	targetVer = &s
- }
+ var targetVer *string // always nil in lint mode

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/lint/command.go` around lines 408 - 413, The IsSet() guard around
c.TargetVersion in formatAndOutputResults is dead (runLintMode only calls this
when TargetVersion.IsSet() is false); remove the conditional block and the
misleading check: either delete the local targetVer variable entirely and update
any call sites to pass nil directly, or explicitly set targetVer to nil (e.g.,
declare var targetVer *string = nil) so the code clearly reflects that
formatAndOutputResults is called with no target version; update references to
targetVer accordingly. Reference symbols: formatAndOutputResults, runLintMode,
c.TargetVersion, targetVer.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

pkg/lint/command.go (1)

466-467: Minor: Consider inlining the string conversion.

The temporary variable targetVerStr exists solely to take its address. This is correct and necessary in Go, but could be documented or simplified if version.SemVersion exposed a StringPtr() method. Not blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/lint/command.go` around lines 466 - 467, The temporary variable
targetVerStr is only created to take its address; add a StringPtr() method on
version.SemVersion that returns *string (e.g., computes s := v.String(); return
&s) and replace the current two-line pattern (targetVerStr :=
c.TargetVersion.String(); targetVer := &targetVerStr) with a single call
targetVer := c.TargetVersion.StringPtr(), or alternatively keep the current code
but add a brief comment above targetVerStr explaining why the temporary is
necessary.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

pkg/migrate/command_options.go (1)

90-95: ⚠️ Potential issue | 🟠 Major

Wrap registry creation errors to satisfy wrapcheck.
Returning the external error unwrapped will fail CI.

🛠️ Suggested fix

 func newDefaultActionRegistry() (*action.ActionRegistry, error) {
-	return action.NewActionRegistry(
+	registry, err := action.NewActionRegistry(
 		&rhbok.RHBOKMigrationAction{},
 	)
+	if err != nil {
+		return nil, fmt.Errorf("create default action registry: %w", err)
+	}
+	return registry, nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/migrate/command_options.go` around lines 90 - 95,
newDefaultActionRegistry currently returns the external error from
action.NewActionRegistry directly; capture the error returned by
action.NewActionRegistry when creating the *action.ActionRegistry (called in
newDefaultActionRegistry with rhbok.RHBOKMigrationAction{}) and wrap it using
fmt.Errorf with the %w verb (or equivalent errors wrapping) before returning so
the error is properly wrapped to satisfy wrapcheck.

pkg/migrate/action/registry.go (1)

30-42: ⚠️ Potential issue | 🟠 Major

Detect duplicate IDs inside the same Register() call.
This still allows duplicates within the batch to overwrite, despite the “atomic” intent.

🛠️ Suggested fix

 func (r *ActionRegistry) Register(actions ...Action) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
+	seen := make(map[string]struct{}, len(actions))
 	for _, a := range actions {
-		if _, exists := r.actions[a.ID()]; exists {
-			return fmt.Errorf("action with ID %q already registered", a.ID())
+		id := a.ID()
+		if _, exists := r.actions[id]; exists {
+			return fmt.Errorf("action with ID %q already registered", id)
+		}
+		if _, exists := seen[id]; exists {
+			return fmt.Errorf("action with ID %q already registered", id)
 		}
+		seen[id] = struct{}{}
 	}
 
 	for _, a := range actions {
 		r.actions[a.ID()] = a
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/migrate/action/registry.go` around lines 30 - 42, ActionRegistry.Register
currently only checks collisions against the existing r.actions map, allowing
duplicate IDs within the same actions ... variadic batch to overwrite; change
Register to first validate the entire batch atomically by scanning the provided
actions and building a temporary seen map (keyed by a.ID()) to detect duplicates
among the batch and also check each ID against r.actions before acquiring the
write into r.actions; if any duplicate (either within the batch or against
existing entries) is found return an error and do not modify r.actions. Ensure
you reference ActionRegistry.Register, r.actions and Action.ID() when locating
the code to implement the temporary seen map and pre-check logic.

docs/lint/writing-checks.md (1)

136-151: ⚠️ Potential issue | 🟡 Minor

Stale workload count in the documentation example.

The abbreviated example shows // Workloads (13), but pkg/lint/command.go now registers 19 workload checks (lines 90–108). // Components (13) and // Dependencies (2) are correct.

📝 Proposed fix

-        // Workloads (13)
+        // Workloads (19)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/lint/writing-checks.md` around lines 136 - 151, The inline comment
counting workload checks is stale: update the comment in the example registry
call so "// Workloads (13)" reflects the current number of workload checks (19).
In the check.NewRegistry(...) example where workload entries like
guardrails.NewOtelMigrationCheck(),
kserveworkloads.NewAcceleratorMigrationCheck(), and
notebook.NewImpactedWorkloadsCheck() are listed, change the workload count to 19
to match the actual registrations in pkg/lint/command.go.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

pkg/migrate/action/registry.go (1)

30-42: ⚠️ Potential issue | 🟠 Major

Handle duplicate IDs within the same Register batch.

Register only checks existing registry entries, so duplicates inside actions will silently overwrite in the second loop, contradicting the “atomic” guarantee and risking hard‑to‑trace behavior.

🔧 Suggested fix

 func (r *ActionRegistry) Register(actions ...Action) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
+	seen := make(map[string]struct{}, len(actions))
 	for _, a := range actions {
-		if _, exists := r.actions[a.ID()]; exists {
-			return fmt.Errorf("action with ID %q already registered", a.ID())
+		id := a.ID()
+		if _, exists := r.actions[id]; exists {
+			return fmt.Errorf("action with ID %q already registered", id)
+		}
+		if _, exists := seen[id]; exists {
+			return fmt.Errorf("action with ID %q already registered", id)
 		}
+		seen[id] = struct{}{}
 	}
 
 	for _, a := range actions {
 		r.actions[a.ID()] = a
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/migrate/action/registry.go` around lines 30 - 42, The Register method in
ActionRegistry currently only checks existing registry entries and can allow
duplicate IDs inside the incoming actions slice to overwrite each other; change
Register (the method on ActionRegistry) to validate the entire batch before
mutating r.actions: while holding r.mu, first iterate the incoming actions and
build a local seen set to detect duplicates inside the batch and also check
against r.actions for conflicts, returning an error if any duplicate is found,
and only after successful validation iterate again to insert each action into
r.actions so the operation is atomic.

pkg/lint/check/registry.go (1)

31-46: Intra-batch duplicate IDs still not detected — atomicity contract is broken for same-batch duplicates.

The first validation loop checks only against existing r.checks, so calling Register(checkA, checkA) (or NewRegistry(checkA, checkA)) passes validation silently and the second check silently overwrites the first. The documented "atomic: either all or none" guarantee is not upheld for duplicates within the same batch.

🛠️ Suggested fix

 func (r *CheckRegistry) Register(checks ...Check) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()

+	seen := make(map[string]struct{}, len(checks))
 	for _, c := range checks {
+		id := c.ID()
+		if _, exists := seen[id]; exists {
+			return fmt.Errorf("check with ID %s already registered", id)
+		}
+		seen[id] = struct{}{}
-		if _, exists := r.checks[c.ID()]; exists {
-			return fmt.Errorf("check with ID %s already registered", c.ID())
+		if _, exists := r.checks[id]; exists {
+			return fmt.Errorf("check with ID %s already registered", id)
 		}
 	}

 	for _, c := range checks {
 		r.checks[c.ID()] = c
 	}

 	return nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/lint/check/registry.go` around lines 31 - 46, Register currently only
checks IDs against r.checks and misses duplicates inside the incoming checks
slice, breaking the atomic "all or none" guarantee; update
CheckRegistry.Register to first validate all incoming checks for duplicates by
building a temporary set (e.g., map[string]struct{}) while holding the lock,
checking each c.ID() both against r.checks and the temp set, and if any
duplicate is found return an error without mutating r.checks; only after the
validation loop succeeds, iterate and insert each check into r.checks so the
operation remains atomic.

docs/lint/writing-checks.md (1)

146-150: ⚠️ Potential issue | 🟡 Minor

// Workloads (13) comment is stale — actual count is 19.

pkg/lint/command.go registers // Workloads (19) with 19 entries. The illustrative sample here still shows 13.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/lint/writing-checks.md` around lines 146 - 150, Update the stale comment
"Workloads (13)" to the correct count "Workloads (19)" so the docs match
pkg/lint/command.go; locate the block containing
guardrails.NewOtelMigrationCheck(),
kserveworkloads.NewAcceleratorMigrationCheck(),
notebook.NewImpactedWorkloadsCheck() and change the leading comment to
"Workloads (19)" (or make it dynamic/consistent with the source if preferred).

docs/lint/architecture.md (1)

85-113: ⚠️ Potential issue | 🟡 Minor

Code sample reintroduces stale check names and workload count previously fixed.

The following diverge from the actual pkg/lint/command.go:

• Line 88: codeflare.NewRemovalCheck() — actual: raycomponent.NewCodeFlareRemovalCheck()
• Line 94: kserve.NewInferenceServiceConfigCheck() — this is now registered as a workload check, not a component check
• Line 104: // Workloads (8) — actual: // Workloads (19)

These were previously flagged and marked as addressed; the current PR's ~-marked edits to this section appear to have reintroduced the old content.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/lint/architecture.md` around lines 85 - 113, This diff reintroduces
stale check names and incorrect grouping: replace the outdated call
codeflare.NewRemovalCheck() with the correct
raycomponent.NewCodeFlareRemovalCheck() and move
kserve.NewInferenceServiceConfigCheck() from the Components list into the
Workloads list so it is registered as a workload check; also update the
Workloads comment/count (e.g., change "// Workloads (8)" to the current "//
Workloads (19)" or the accurate number) to match pkg/lint/command.go and ensure
the registry initialization (the call to check.NewRegistry with the listed
New*Check() entries) reflects the canonical set and ordering.

pkg/migrate/command_prepare.go (1)

29-31: PrepareCommand retains the old TargetVersion string + parsedTargetVersion *semver.Version pattern.

This PR introduces version.SemVersion for the lint command's TargetVersion, eliminating the manual parse-in-Complete() step and validating at flag-parse time. PrepareCommand still uses fs.StringVar + semver.ParseTolerant in Complete(), which is now inconsistent across the codebase.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/migrate/command_prepare.go` around lines 29 - 31, PrepareCommand
currently keeps TargetVersion as string and parsedTargetVersion *semver.Version;
instead switch TargetVersion to use version.SemVersion (the new flag-validated
type), remove parsedTargetVersion, stop calling semver.ParseTolerant in
Complete(), and register the flag with fs.Var(&c.TargetVersion, ...) or the
appropriate fs helper so the value is validated at parse time; update references
in PrepareCommand.Complete() to use c.TargetVersion (already parsed) and delete
the manual parsing/validation code paths.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting